-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 don't consider bundle deprecated if the package is deprecated #577
🌱 don't consider bundle deprecated if the package is deprecated #577
Conversation
if dep.Reference.Schema == declcfg.SchemaPackage && dep.Reference.Name == b.Package { | ||
return true | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this will actually mean that explicitly deprecated bundles will be preferred less than ones that aren't even if the entire package is deprecated - fixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this a little closer (sorry I didn't get the chance with the original PR), I'm a little concerned that we're putting package and channel level deprecations in the Bundle
type.
I think I would have expected that we would put the package deprecation in the Package
type, channel deprecation in the Channel
type and bundle deprecation in the Bundle
type.
And if we had that, we could use composition by embedding the deprecation type in each of the package/channel/bundle types and then use a shared IsDeprecated
function that comes with that deprecation type.
Is it possible to do something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, but my concern with doing that is that we would likely need to restructure resolution to provide all those types as results from the resolution. IIUC the resolution process currently will only return the Bundle
type that contains all the necessary information for continuing with the install/upgrade process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restructuring resolution to return variables for Package
, Channel
, and Bundle
types is out of scope for this work IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this will actually mean that explicitly deprecated bundles will be preferred less than ones that aren't even if the entire package is deprecated - fixing
One other question about this point. Think about this scenario:
- Package
foo
is deprecated - Bundle
foo.v1.0.0
is not deprecated - Bundle
foo.v1.0.1
is deprecated
I would expect foo.v1.0.0
to be preferred over foo.v1.0.1
. The package
deprecation should not cascade to the bundles one way or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay. I was under the impression that in that scenario foo.v1.0.1
would still be preferred over foo.v1.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6fdc060 - reverts to where the check is removed and should satisfy the expectation you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford Could you take another look when you have some time?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
+ Coverage 84.35% 84.83% +0.48%
==========================================
Files 20 20
Lines 933 930 -3
==========================================
+ Hits 787 789 +2
+ Misses 102 98 -4
+ Partials 44 43 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ccb4a31
to
6f651c7
Compare
Signed-off-by: everettraven <everettraven@gmail.com>
7ee8dc9
to
1ab95bb
Compare
Signed-off-by: everettraven <everettraven@gmail.com>
}, | ||
{ | ||
name: "has no deprecation entries, not deprecated", | ||
bundle: &catalogmetadata.Bundle{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly a silly question: Does this catalogmetadata.Bundle{}
need to have the package and channel deprecations listed for them to show up in the ClusterExtension deprecation status conditions, or is that handled in a separate data structure?
If it is handled in a separate data strucutre, could we change catalogmetadata.Bundle
to contain something like:
package catalogmetadata
type Bundle struct {
...
deprecated bool
}
It seems unnecessary to store a list of Deprecations
where there will always be 0 or 1 and the schema/name will always be olm.bundle
and bundle.Name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this catalogmetadata.Bundle{} need to have the package and channel deprecations listed for them to show up in the ClusterExtension deprecation status conditions
Correct.
or is that handled in a separate data structure?
Not handled in a separate data structure at the moment as the catalogmetadata.Bundle type is what is used to determine everything after resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this question is related to #577 (comment)
Description
catalogmetadata.Bundle.IsDeprecated()
method to no longer return true if there is a package deprecation associated with the bundlecatalogmetadata.Bundle.IsDeprecated()
methodReviewer Checklist